Skip to content

Move type dependencies to peer dependencies. #113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

ericmackrodt
Copy link

@ericmackrodt ericmackrodt commented Jul 10, 2019

What:

Moves the @types from dependencies to peerDependencies in the package.json.

Why:

Because the dependencies are always installed, if the project this library is being used already has references to those types but in a different version, especially if you're using Lerna, you get multiple Duplicate identifier errors in typescript.

How:

The dependencies were moved to peerDependencies and also added to devDependencies for the library development.

Checklist:

  • Documentation updated
  • Tests
  • Typescript definitions updated
  • Ready to be merged
  • Added myself to contributors table

@codecov-io
Copy link

codecov-io commented Jul 10, 2019

Codecov Report

Merging #113 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #113   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          39     39           
  Branches        3      3           
=====================================
  Hits           39     39

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 396049b...26b7c55. Read the comment docs.

@mpeyper
Copy link
Member

mpeyper commented Jul 14, 2019

This has been discussed a bit in #110 already, but I'm not convinced the type dependencies are actually wrong here. Because of semver versioning, the types should resolve to a single version, so long as they are compatible semver ranges.

In the case of lerna, npm link (which lerna uses under the hood) is well documented as causing issues when resolving type definitions.

It's an annoying situation for me because either I make the type definitions a dependency and have it install the right one for all the cases when someone doesn't care about they type definition (i.e. non-typescript user or typescript user without that dependency themselves), or I have them as peerDependencies and make everyone who doesn't care about them get a peerDependency warning on installation (less important for a typescript user without that dependency, as they should be installing it anyone, but a pain for a non-typescript user).

@mpeyper
Copy link
Member

mpeyper commented Aug 9, 2019

Closing this. See #110 for details.

@mpeyper mpeyper closed this Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants